-
Notifications
You must be signed in to change notification settings - Fork 889
Added fixer for newline-before-return rule #4482
Added fixer for newline-before-return rule #4482
Conversation
Thanks for your interest in palantir/tslint, @jukben! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request. |
a27f247
to
f9f4e7d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great idea, thanks for getting started on this! Some touchup suggestions around sharing code with existing rules that do similar things.
src/rules/newlineBeforeReturnRule.ts
Outdated
const fixer = Lint.Replacement.replaceFromTo( | ||
start - indentationCurrent.length, | ||
start, | ||
line === prevLine ? `\n\n${indentationPrev}` : `\n${indentationCurrent}`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
\n
This'll need to adjust to projects where \r\n
is the line feed, such as Git with CRLF (aka Windows). curlyRule.ts
checks the last character of a relevant source line to see which to use, for example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a very good point, haven't thought about it. I've introduced function in utils to solve that. 👍
Hey, @JoshuaKGoldberg thank you for the feedback, wouldn't you mind do me another review? 🙏 |
36c53b1
to
8aaec1c
Compare
8aaec1c
to
37646c5
Compare
@jukben have you signed the CLA? also please avoid force-pushing to PRs in this project, it's easier to review if you just add additional commits. they all get squashed at the end of the PR. |
@adidahiya yes I have signed the CLA already.
Anyway, sorry for the force-push I’m aware of this, I haven’t been adding new commits I was just trying to unblock/resolve this because of conflicts coming from the master (#4420). Should I use a merge strategy next time? P.S: I already had that check green, maybe rerun it would do the trick? |
@jukben yes you can use a merge strategy from master. hmm it looks like our CLA bot might be buggy, it also failed to report status on one of my PRs recently. would you mind posting a screenshot of where you saw that text "you signed the individual CLA..."? thanks |
@adidahiya Makes sense, next time I'll do it like this to avoid any force-push when PR is being reviewed. 👍 Is this enough? 🙏 |
Could we please unblock it? @JoshuaKGoldberg @adidahiya |
@adidahiya ❤️ Could I somewhere get the information how releases are planned? Just to plan when it will be possible to intercorporate it cleanly to my project? |
the release schedule is pretty ad-hoc right now, but I'll try to cut a release this week |
PR checklist
Overview of change:
I've realized that newline-before-return didn't have a fixer which seems to be quite straightforward. Here are my two cents.
I've added tests and everything seems OK.
Is there anything you'd like reviewers to focus on?
CHANGELOG.md entry:
[new-fixer] added fixer for newline-before-return rule